Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
On 10/2/23 15:42, Miquel Raynal wrote: Hi Marek, ma...@denx.de wrote on Sat, 30 Sep 2023 23:11:17 +0200: On 9/27/23 15:59, Miquel Raynal wrote: Hi Marek, Hi, miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: Hi Marek, I'm answering here as there is no cover letter. Just to let you know I'm still concerned by the series and want to test it but did not had the time to do so recently. Hopefully next week. The series looks good to me and works as well on a Beagle Bone Black with no visible functional changes regarding the use of the UDC. The whole series is: Tested-by: Miquel Raynal By the way, following your initial series there have been three followup patches trying to improve a little bit the doc, one got merged and two others were delegated to you: https://patchwork.ozlabs.org/project/uboot/list/?series=367635 They are almost 2 months old now, would you mind acking or merging them so both your initial USB gadget rework and the additional (related) doc can be in the same release please? Can you resend those and CC this mail address ? Of course! That said, 1/2 should be in some sort of README instead, and the 'dm tree' command depends on CMD_DM=y . Well, the README is more something which targets the developer IMO, whereas here I want to target the users. Many people only see U-Boot through Yocto now (that's a different topic) and the purpose of this additional text is to help them in finding their way into U-Boot device model *alone*. It's really short, but I bet it can really help, given how I felt when I actually understood why you advised to type a couple of semi-random bind/unbind commands which eventually worked. Anybody not involved in any DM conversion is likely just not aware of all of that. Having people finding their own way through the DM means less complaints/needs for help on the mailing list. Thinking about it back and forth, I think you've got a point, yes. The usb_gadget_probe_driver() is code synchronized from kernel, you likely want to add any such clarification to usb_gadget_register_driver() . Actually the kernel functions do have some kind of alternate error message as well now :-) Clearly different given that this code has been adapted to U-Boot's DM, but the idea is close. However usb_gadget_register_driver() is kind of blind regarding the number of UDCs in the system and if they are already bound or not, so I don't really see the point in moving the error message there along with all the logic duplication involved. That said, the usb_gadget_register_driver() should be reworked into some new functions, which takes UDC controller *udevice pointer to avoid this binding heuristics. Yes, I don't know many boards with two UDCs but it's a clear limitation. i.MX8M Plus is one such example, Quad I think is another, and pretty much anything with more than one DWC3 controller is. The CI HDRC is also often present in OTG capable configuration, so a lot of i.MXes are multi UDC systems.
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, ma...@denx.de wrote on Sat, 30 Sep 2023 23:11:17 +0200: > On 9/27/23 15:59, Miquel Raynal wrote: > > Hi Marek, > > Hi, > > > miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: > > > >> Hi Marek, > >> > >> I'm answering here as there is no cover letter. Just to let you know > >> I'm still concerned by the series and want to test it but did not had > >> the time to do so recently. Hopefully next week. > > > > The series looks good to me and works as well on a Beagle Bone Black > > with no visible functional changes regarding the use of the UDC. The > > whole series is: > > > > Tested-by: Miquel Raynal > > > > By the way, following your initial series there have been three > > followup patches trying to improve a little bit the doc, one got merged > > and two others were delegated to you: > > https://patchwork.ozlabs.org/project/uboot/list/?series=367635 > > > > They are almost 2 months old now, would you mind acking or merging > > them so both your initial USB gadget rework and the additional > > (related) doc can be in the same release please? > > Can you resend those and CC this mail address ? Of course! > That said, 1/2 should be in some sort of README instead, and the 'dm tree' > command depends on CMD_DM=y . Well, the README is more something which targets the developer IMO, whereas here I want to target the users. Many people only see U-Boot through Yocto now (that's a different topic) and the purpose of this additional text is to help them in finding their way into U-Boot device model *alone*. It's really short, but I bet it can really help, given how I felt when I actually understood why you advised to type a couple of semi-random bind/unbind commands which eventually worked. Anybody not involved in any DM conversion is likely just not aware of all of that. Having people finding their own way through the DM means less complaints/needs for help on the mailing list. > The usb_gadget_probe_driver() is code synchronized from kernel, you likely > want to add any such clarification to usb_gadget_register_driver() . Actually the kernel functions do have some kind of alternate error message as well now :-) Clearly different given that this code has been adapted to U-Boot's DM, but the idea is close. However usb_gadget_register_driver() is kind of blind regarding the number of UDCs in the system and if they are already bound or not, so I don't really see the point in moving the error message there along with all the logic duplication involved. > That said, the usb_gadget_register_driver() should be reworked into > some new functions, which takes UDC controller *udevice pointer to > avoid this binding heuristics. Yes, I don't know many boards with two UDCs but it's a clear limitation. Thanks, Miquèl
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
On 9/27/23 15:59, Miquel Raynal wrote: Hi Marek, Hi, miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: Hi Marek, I'm answering here as there is no cover letter. Just to let you know I'm still concerned by the series and want to test it but did not had the time to do so recently. Hopefully next week. The series looks good to me and works as well on a Beagle Bone Black with no visible functional changes regarding the use of the UDC. The whole series is: Tested-by: Miquel Raynal By the way, following your initial series there have been three followup patches trying to improve a little bit the doc, one got merged and two others were delegated to you: https://patchwork.ozlabs.org/project/uboot/list/?series=367635 They are almost 2 months old now, would you mind acking or merging them so both your initial USB gadget rework and the additional (related) doc can be in the same release please? Can you resend those and CC this mail address ? That said, 1/2 should be in some sort of README instead, and the 'dm tree' command depends on CMD_DM=y . The usb_gadget_probe_driver() is code synchronized from kernel, you likely want to add any such clarification to usb_gadget_register_driver() . That said, the usb_gadget_register_driver() should be reworked into some new functions, which takes UDC controller *udevice pointer to avoid this binding heuristics.
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, miquel.ray...@bootlin.com wrote on Fri, 22 Sep 2023 12:00:12 +0200: > Hi Marek, > > I'm answering here as there is no cover letter. Just to let you know > I'm still concerned by the series and want to test it but did not had > the time to do so recently. Hopefully next week. The series looks good to me and works as well on a Beagle Bone Black with no visible functional changes regarding the use of the UDC. The whole series is: Tested-by: Miquel Raynal By the way, following your initial series there have been three followup patches trying to improve a little bit the doc, one got merged and two others were delegated to you: https://patchwork.ozlabs.org/project/uboot/list/?series=367635 They are almost 2 months old now, would you mind acking or merging them so both your initial USB gadget rework and the additional (related) doc can be in the same release please? Thanks, Miquèl
Re: [PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Hi Marek, I'm answering here as there is no cover letter. Just to let you know I'm still concerned by the series and want to test it but did not had the time to do so recently. Hopefully next week. Sorry for the delay. Miquèl ma...@denx.de wrote on Fri, 1 Sep 2023 11:49:47 +0200: > Pull the functionality of UDC uclass that operates on plain udevice > and does not use this dev_array array into separate functions and > expose those functions, so that as much code as possible can be > switched over to these functions and the dev_array can be dropped. > > Reviewed-by: Mattijs Korpershoek > Signed-off-by: Marek Vasut > --- > Cc: Angus Ainslie > Cc: Dmitrii Merkurev > Cc: Eddie Cai > Cc: Kever Yang > Cc: Lukasz Majewski > Cc: Miquel Raynal > Cc: Mattijs Korpershoek > Cc: Nishanth Menon > Cc: Patrice Chotard > Cc: Patrick Delaunay > Cc: Philipp Tomsich > Cc: Simon Glass > Cc: Stefan Roese > Cc: ker...@puri.sm > --- > V2: Add RB from Mattijs > --- > drivers/usb/gadget/udc/Makefile | 2 +- > drivers/usb/gadget/udc/udc-uclass.c | 57 + > include/linux/usb/gadget.h | 17 + > 3 files changed, 68 insertions(+), 8 deletions(-)
[PATCH v2 01/17] dm: usb: udc: Factor out plain udevice handler functions
Pull the functionality of UDC uclass that operates on plain udevice
and does not use this dev_array array into separate functions and
expose those functions, so that as much code as possible can be
switched over to these functions and the dev_array can be dropped.
Reviewed-by: Mattijs Korpershoek
Signed-off-by: Marek Vasut
---
Cc: Angus Ainslie
Cc: Dmitrii Merkurev
Cc: Eddie Cai
Cc: Kever Yang
Cc: Lukasz Majewski
Cc: Miquel Raynal
Cc: Mattijs Korpershoek
Cc: Nishanth Menon
Cc: Patrice Chotard
Cc: Patrick Delaunay
Cc: Philipp Tomsich
Cc: Simon Glass
Cc: Stefan Roese
Cc: ker...@puri.sm
---
V2: Add RB from Mattijs
---
drivers/usb/gadget/udc/Makefile | 2 +-
drivers/usb/gadget/udc/udc-uclass.c | 57 +
include/linux/usb/gadget.h | 17 +
3 files changed, 68 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile
index 95dbf0c82ee..467c566f6d3 100644
--- a/drivers/usb/gadget/udc/Makefile
+++ b/drivers/usb/gadget/udc/Makefile
@@ -7,4 +7,4 @@ obj-$(CONFIG_USB_DWC3_GADGET) += udc-core.o
endif
obj-$(CONFIG_$(SPL_)DM_USB_GADGET) += udc-core.o
-obj-$(CONFIG_$(SPL_)DM) += udc-uclass.o
+obj-y += udc-uclass.o
diff --git a/drivers/usb/gadget/udc/udc-uclass.c
b/drivers/usb/gadget/udc/udc-uclass.c
index de8861829c7..b4271b4be9f 100644
--- a/drivers/usb/gadget/udc/udc-uclass.c
+++ b/drivers/usb/gadget/udc/udc-uclass.c
@@ -14,6 +14,37 @@
#if CONFIG_IS_ENABLED(DM_USB_GADGET)
#define MAX_UDC_DEVICES 4
static struct udevice *dev_array[MAX_UDC_DEVICES];
+
+int udc_device_get_by_index(int index, struct udevice **udev)
+{
+ struct udevice *dev = NULL;
+ int ret;
+
+ ret = uclass_get_device_by_seq(UCLASS_USB_GADGET_GENERIC, index, &dev);
+ if (!ret && dev) {
+ *udev = dev;
+ return 0;
+ }
+
+ ret = uclass_get_device(UCLASS_USB_GADGET_GENERIC, index, &dev);
+ if (!ret && dev) {
+ *udev = dev;
+ return 0;
+ }
+
+ pr_err("No USB device found\n");
+ return -ENODEV;
+}
+
+int udc_device_put(struct udevice *udev)
+{
+#if CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)
+ return device_remove(udev, DM_REMOVE_NORMAL);
+#else
+ return -ENOSYS;
+#endif
+}
+
int usb_gadget_initialize(int index)
{
int ret;
@@ -23,13 +54,10 @@ int usb_gadget_initialize(int index)
return -EINVAL;
if (dev_array[index])
return 0;
- ret = uclass_get_device_by_seq(UCLASS_USB_GADGET_GENERIC, index, &dev);
+ ret = udc_device_get_by_index(index, &dev);
if (!dev || ret) {
- ret = uclass_get_device(UCLASS_USB_GADGET_GENERIC, index, &dev);
- if (!dev || ret) {
- pr_err("No USB device found\n");
- return -ENODEV;
- }
+ pr_err("No USB device found\n");
+ return -ENODEV;
}
dev_array[index] = dev;
return 0;
@@ -42,7 +70,7 @@ int usb_gadget_release(int index)
if (index < 0 || index >= ARRAY_SIZE(dev_array))
return -EINVAL;
- ret = device_remove(dev_array[index], DM_REMOVE_NORMAL);
+ ret = device_remove(dev_array[index]);
if (!ret)
dev_array[index] = NULL;
return ret;
@@ -57,10 +85,25 @@ int usb_gadget_handle_interrupts(int index)
return -EINVAL;
return dm_usb_gadget_handle_interrupts(dev_array[index]);
}
+#else
+/* Backwards hardware compatibility -- switch to DM_USB_GADGET */
+static int legacy_index;
+int udc_device_get_by_index(int index, struct udevice **udev)
+{
+ legacy_index = index;
+ return board_usb_init(index, USB_INIT_DEVICE);
+}
+
+int udc_device_put(struct udevice *udev)
+{
+ return board_usb_cleanup(legacy_index, USB_INIT_DEVICE);
+}
#endif
+#if CONFIG_IS_ENABLED(DM)
UCLASS_DRIVER(usb_gadget_generic) = {
.id = UCLASS_USB_GADGET_GENERIC,
.name = "usb",
.flags = DM_UC_FLAG_SEQ_ALIAS,
};
+#endif
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 2f694fc25c1..5e9a6513d5b 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1006,6 +1006,23 @@ extern void usb_ep_autoconfig_reset(struct usb_gadget *);
extern int usb_gadget_handle_interrupts(int index);
+/**
+ * udc_device_get_by_index() - Get UDC udevice by index
+ * @index: UDC device index
+ * @udev: UDC udevice matching the index (if found)
+ *
+ * Return: 0 if Ok, -ve on error
+ */
+int udc_device_get_by_index(int index, struct udevice **udev);
+
+/**
+ * udc_device_put() - Put UDC udevice
+ * @udev: UDC udevice
+ *
+ * Return: 0 if Ok, -ve on error
+ */
+int udc_device_put(struct udevice *udev);
+
#if CONFIG_IS_ENABLED(DM_USB_GADGET)
int usb_gadget_initialize(int index);
int usb_gadget_release(int index);
--
2.40.1
