usb: driver-model fixes and dm support sunxi-ehci.c
Subsystems (3)
cover usb: driver-model fixes and dm support sunxi-ehci.c
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
PATCH 1/8 dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device
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.
--- drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- include/usb.h | 5 +---- 2 files changed, 7 insertions(+), 25 deletions(-)
drivers/usb/host/usb-uclass.c +6 -21
@@ -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 ; }
include/usb.h +1 -4
@@ -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 */
Signature · 1 line
2.3.6
PATCH 1/8 dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device
Hi Hans,
Quoted 10 lines · Hans de Goede
Currently we copy over a number of
usb_devicevalues stored in the on stack structusb_deviceprobed inusb_scan_device()to the final driver-model managed structusb_deviceinusb_child_pre_probe()throughusb_device_platdata, and then callusb_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?
Quoted 3 lines
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?
Quoted 79 lines
This commit fixes both issues by removing the
usb_select_config()call fromusb_child_pre_probe(), and instead of copying over things field by field throughusb_device_platdata, store a pointer to the in stackusb_device(which is still valid whenusb_child_pre_probe()gets called) and copy over the entire struct.Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- include/usb.h | 5 +---- 2 files changed, 7 insertions(+), 25 deletions(-)drivers/usb/host/usb-uclass.c +6
-21@@ -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 ; }include/usb.h +1
-5@@ -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 */ --
Quoted 2 lines
2.3.6
Regards,
Simon
PATCH 1/8 dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device
Hi,
Quoted 8 lines · Simon Glass
Hi Hans,
Quoted 5 lines · Hans de Goede
Currently we copy over a number of
usb_devicevalues stored in the on stack structusb_deviceprobed inusb_scan_device()to the final driver-model managed structusb_deviceinusb_child_pre_probe()throughusb_device_platdata, and then callusb_select_config()to fill in the rest.
Quoted 7 lines
Quoted 5 lines
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 :)
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
The extra requests are in the second call to
usb_select_config().
Correct.
Quoted 2 lines
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
Quoted 86 lines
Quoted 80 lines
This commit fixes both issues by removing the
usb_select_config()call fromusb_child_pre_probe(), and instead of copying over things field by field throughusb_device_platdata, store a pointer to the in stackusb_device(which is still valid whenusb_child_pre_probe()gets called) and copy over the entire struct.Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/usb-uclass.c | 27 ++++++--------------------- include/usb.h | 5 +---- 2 files changed, 7 insertions(+), 25 deletions(-)drivers/usb/host/usb-uclass.c +6
-21@@ -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 ; }include/usb.h +1
-5@@ -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 */ --Quoted 2 lines
2.3.6
Regards,
Simon
PATCH 1/8 dm: usb: Copy over usb_device values from usb_scan_device() to final usb_device
Hi,
Quoted 36 lines · Hans de Goede
Hi,
Quoted 8 lines · Simon Glass
Hi Hans,
Quoted 5 lines · Hans de Goede
Currently we copy over a number of
usb_devicevalues stored in the on stack structusb_deviceprobed inusb_scan_device()to the final driver-model managed structusb_deviceinusb_child_pre_probe()throughusb_device_platdata, and then callusb_select_config()to fill in the rest.Quoted 7 lines
Quoted 5 lines
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 :)
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
The extra requests are in the second call to
usb_select_config().Correct.
Quoted 2 lines
Do you think we could put the things we want to copy in a struct, and copy just those?
Quoted 6 lines
We would end up pretty much duplicating
usb_deviceAFAICT, 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
PATCH 2/8 dm: usb: Use controller_dev in dm ehci code
In reply to Hans de GoedeUse the controller_dev pointer in the ehci hcd code rather then going up the tree till we find the first UCLASS_USB device.
--- drivers/usb/host/ehci-hcd.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/usb/host/ehci-hcd.c +1 -8
@@ -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
Signature · 1 line
2.3.6
PATCH 2/8 dm: usb: Use controller_dev in dm ehci code
Hi Hans,
Quoted 18 lines · Hans de Goede
Use the
controller_devpointer in the ehci hcd code rather then going up the tree till we find the firstUCLASS_USBdevice.Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/ehci-hcd.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)drivers/usb/host/ehci-hcd.c +0
-2@@ -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; -
drivers/usb/host/ehci-hcd.c static struct descriptor { · line 131
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +1
-6-/* 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
PATCH 2/8 dm: usb: Use controller_dev in dm ehci code
Hi,
Quoted 27 lines · Simon Glass
Hi Hans,
Quoted 24 lines · Hans de Goede
Use the
controller_devpointer in the ehci hcd code rather then going up the tree till we find the firstUCLASS_USBdevice.Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/ehci-hcd.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)drivers/usb/host/ehci-hcd.c +0
-8@@ -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);
Quoted 8 lines
drivers/usb/host/ehci-hcd.c static struct descriptor { · line 131
Quoted 4 lines
drivers/usb/host/ehci-hcd.c +1
-0+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
PATCH 3/8 dm: usb: Store usb_device parent pointer in usb_device
In reply to Hans de GoedeWhen 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.
--- 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(-)
drivers/usb/host/ehci-hcd.c +1 -23
@@ -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
drivers/usb/host/usb-uclass.c +2 -3
@@ -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;
include/usb.h +1 -1
@@ -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
Signature · 1 line
2.3.6
PATCH 3/8 dm: usb: Store usb_device parent pointer in usb_device
Hi Hans,
Quoted 16 lines · 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_devicepointer to structusb_deviceso that the usb hcd code can get to theusb_deviceparent without needing to go through the dm.This fixes usb-1 devices plugged into usb-2 hubs not working with the driver model.
Quoted 7 lines
Signed-off-by Hans de Goede <[email protected]>--- 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?
Quoted 91 lines
drivers/usb/host/ehci-hcd.c +1
-23@@ -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 intdrivers/usb/host/usb-uclass.c +2
-3@@ -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;include/usb.h +1
-2@@ -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
PATCH 3/8 dm: usb: Store usb_device parent pointer in usb_device
Hi,
Quoted 27 lines · Simon Glass
Hi Hans,
Quoted 16 lines · 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_devicepointer to structusb_deviceso that the usb hcd code can get to theusb_deviceparent without needing to go through the dm.This fixes usb-1 devices plugged into usb-2 hubs not working with the driver model.
Quoted 7 lines
Signed-off-by Hans de Goede <[email protected]>--- 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(-)
Quoted 7 lines
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
Quoted 91 lines
Quoted 91 lines
drivers/usb/host/ehci-hcd.c +1
-23@@ -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 intdrivers/usb/host/usb-uclass.c +2
-3@@ -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;include/usb.h +1
-2@@ -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
PATCH 4/8 dm: usb: Set desc_before_addr from ehci dm code
In reply to Hans de GoedeWithout this usb-1 device descriptors do not get read properly.
--- drivers/usb/host/ehci-hcd.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/usb/host/ehci-hcd.c +3 -0
@@ -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;
Signature · 1 line
2.3.6
PATCH 4/8 dm: usb: Set desc_before_addr from ehci dm code
Quoted 6 lines · Hans de Goede
Without this usb-1 device descriptors do not get read properly.
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/ehci-hcd.c | 3 +++ 1 file changed, 3 insertions(+)
Quoted 24 lines
drivers/usb/host/ehci-hcd.c +3
-1@@ -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
PATCH 5/8 dm: usb: Add support for interrupt queues to the dm usb code
In reply to Hans de GoedeInterrupt 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.
--- drivers/usb/host/usb-uclass.c | 36 ++++++++++++++++++++++++++++++++ include/usb.h | 48 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-)
drivers/usb/host/usb-uclass.c +36 -0
@@ -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;
include/usb.h +47 -1
60 lines not on this page · raw message
Signature · 1 line
2.3.6
PATCH 5/8 dm: usb: Add support for interrupt queues to the dm usb code
Quoted 7 lines · 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.
Quoted 7 lines
See the added doc comments for more details.
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/usb-uclass.c | 36 ++++++++++++++++++++++++++++++++ include/usb.h | 48 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-)
PATCH 6/8 dm: usb: Prefix ehci interrupt-queue functions with _ehci_
In reply to Hans de GoedeThis is a preparation patch for adding interrupt-queue support to the ehci dm code.
--- drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
drivers/usb/host/ehci-hcd.c +25 -6
@@ -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
Signature · 1 line
2.3.6
PATCH 6/8 dm: usb: Prefix ehci interrupt-queue functions with _ehci_
Quoted 7 lines · Hans de Goede
This is a preparation patch for adding interrupt-queue support to the ehci dm code.
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)
PATCH 7/8 dm: usb: Add support for interrupt queues to the dm ehci code
In reply to Hans de GoedeAdd support for interrupt queues to the dm ehci code.
--- drivers/usb/host/ehci-hcd.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
drivers/usb/host/ehci-hcd.c +26 -0
@@ -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
Signature · 1 line
2.3.6
PATCH 7/8 dm: usb: Add support for interrupt queues to the dm ehci code
Quoted 6 lines · Hans de Goede
Add support for interrupt queues to the dm ehci code.
Signed-off-by Hans de Goede <[email protected]>--- drivers/usb/host/ehci-hcd.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
PATCH 8/8 sunxi: ehci: Convert to the driver-model
In reply to Hans de GoedeConvert sunxi-boards which use the sunxi-ehci code to the driver-model.
--- board/sunxi/Kconfig | 3 ++ drivers/usb/host/ehci-sunxi.c | 95 ++++++++++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 29 deletions(-)
board/sunxi/Kconfig +3 -0
@@ -559,4 +559,7 @@ config DM_ETH config DM_SERIAL default y +config DM_USB + default y if !USB_MUSB_SUNXI + endif
drivers/usb/host/ehci-sunxi.c +66 -29
119 lines not on this page · raw message
Signature · 1 line
2.3.6
PATCH 8/8 sunxi: ehci: Convert to the driver-model
Hi Hans,
Quoted 7 lines · Hans de Goede
Convert sunxi-boards which use the sunxi-ehci code to the driver-model.
Signed-off-by Hans de Goede <[email protected]>--- board/sunxi/Kconfig | 3 ++ drivers/usb/host/ehci-sunxi.c | 95 ++++++++++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 29 deletions(-)
A few nits, but otherwise:
Quoted 18 lines
board/sunxi/Kconfig +3
-0@@ -559,4 +559,7 @@ config DM_ETH config DM_SERIAL default y +config DM_USB + default y if !USB_MUSB_SUNXI + endifdrivers/usb/host/ehci-sunxi.c +0
-0
drivers/usb/host/ehci-sunxi.c line 21
Quoted 10 lines
drivers/usb/host/ehci-sunxi.c +3
-2@@ -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: ?
drivers/usb/host/ehci-sunxi.c line 28
Quoted 7 lines
drivers/usb/host/ehci-sunxi.c +7
-0+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?
Quoted 103 lines
drivers/usb/host/ehci-sunxi.c +56
-28+} + +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 , +}; --
Quoted 2 lines
2.3.6
Regards,
Simon
PATCH 8/8 sunxi: ehci: Convert to the driver-model
Hi,
Quoted 46 lines · Simon Glass
Hi Hans,
Quoted 7 lines · Hans de Goede
Convert sunxi-boards which use the sunxi-ehci code to the driver-model.
Signed-off-by Hans de Goede <[email protected]>--- 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 <[email protected]>Quoted 18 lines
board/sunxi/Kconfig +3
-0@@ -559,4 +559,7 @@ config DM_ETH config DM_SERIAL default y +config DM_USB + default y if !USB_MUSB_SUNXI + endifdrivers/usb/host/ehci-sunxi.c +0
-0drivers/usb/host/ehci-sunxi.c line 21
Quoted 10 lines
drivers/usb/host/ehci-sunxi.c +3
-2@@ -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: ?
Quoted 9 lines
drivers/usb/host/ehci-sunxi.c line 28
Quoted 7 lines
drivers/usb/host/ehci-sunxi.c +7
-0+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
Quoted 110 lines
Quoted 104 lines
drivers/usb/host/ehci-sunxi.c +56
-28+} + +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 , +}; --Quoted 2 lines
2.3.6
Regards,
Simon
PATCH 8/8 sunxi: ehci: Convert to the driver-model
In reply to Hans de Goededrivers/usb/host/ehci-sunxi.c line 41
Quoted 3 lines · Hans de Goede
drivers/usb/host/ehci-sunxi.c +3
-0+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.
PATCH 8/8 sunxi: ehci: Convert to the driver-model
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.
Quoted 15 lines · Hans de Goede
drivers/usb/host/ehci-sunxi.c line 41
Quoted 3 lines
drivers/usb/host/ehci-sunxi.c +3
-0+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.
PATCH 8/8 sunxi: ehci: Convert to the driver-model
In reply to Ian CampbellHi,
Quoted 10 lines · Ian Campbell
drivers/usb/host/ehci-sunxi.c line 41
Quoted 3 lines · Hans de Goede
drivers/usb/host/ehci-sunxi.c +3
-0+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
cover usb: driver-model fixes and dm support sunxi-ehci.c
In reply to Hans de GoedeHi,
Quoted 9 lines · 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.
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"
cover usb: driver-model fixes and dm support sunxi-ehci.c
In reply to Hans de GoedeHi Hans,
Quoted 9 lines · 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.
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
cover usb: driver-model fixes and dm support sunxi-ehci.c
Hi,
Quoted 15 lines · Simon Glass
Hi Hans,
Quoted 9 lines · 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.
Great to see this, thanks for all the work and fixes.
Quoted 7 lines
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
cover usb: driver-model fixes and dm support sunxi-ehci.c
Hi Hans,
Quoted 31 lines · Hans de Goede
Hi,
Quoted 18 lines · Simon Glass
Hi Hans,
Quoted 10 lines · 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.
Great to see this, thanks for all the work and fixes.
Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 6 lines
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.
Quoted 9 lines
What you've been doing sofar is in essence passing through struct
usb_devicefromusb_scan_deviceto thepre_child_probemethod, 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 ofusb_deviceinto theplat_datapassed fromsb_scan_deviceto thepre_child_probe, but I decided it would be easier to just pass all of theusb_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.
Quoted 6 lines
Another slightly cleaner (IMHO) option then copying over the entire
usb_devicewould be to dynamically allocusb_deviceinusb_scan_device, and keep using the sameusb_deviceall 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 inusb_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.
Quoted 3 lines
This effectively gives us the kernel model of allocating the generic
usb_devicebits 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
cover usb: driver-model fixes and dm support sunxi-ehci.c
Hi,
Quoted 34 lines · Simon Glass
Hi Hans,
Quoted 31 lines · Hans de Goede
Hi,
Quoted 25 lines · Simon Glass
Hi Hans,
Quoted 10 lines · 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.
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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 17 lines
Quoted 7 lines
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.
Quoted 9 lines
What you've been doing sofar is in essence passing through struct
usb_devicefromusb_scan_deviceto thepre_child_probemethod, 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 ofusb_deviceinto theplat_datapassed fromsb_scan_deviceto thepre_child_probe, but I decided it would be easier to just pass all of theusb_device
Quoted 4 lines
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.
Quoted 6 lines
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.Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Another slightly cleaner (IMHO) option then copying over the entire
usb_devicewould be to dynamically allocusb_deviceinusb_scan_device, and keep using the sameusb_deviceall 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 inusb_scan_device.But then we have a struct
usb_devicebefore 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.
Quoted 5 lines
Quoted 4 lines
This effectively gives us the kernel model of allocating the generic
usb_devicebits before looking for a driver.
Quoted 7 lines
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