[PATCH 0/3] Support MSM8916 USB PHY + UDC in qcom_defconfig

[PATCH 0/3] Support MSM8916 USB PHY + UDC in qcom_defconfig

[Sam Day]

The MSM8916 SoC includes a ChipIdea UDC. The U-Boot driver for this UDC
is quite old, and did not support USB gadget driver model. As a result,
it could not be built alongside the DWC3 driver. This meant that
qcom_defconfig could not include support for USB on MSM8916.

This series adds support to build ChipIdea UDC with DM_USB_GADGET and
ensures that the newly introduced driver is registered from the MSM EHCI
glue driver.

The ChipIdea UDC driver continues to work without DM_USB_GADGET. I
tested these changes by building dragonboard410c_defconfig (which does
not enable DM_USB_GADGET), booting it, and then chain-booting (via
fastboot) another u-boot build built with qcom_defconfig. USB gadget
mode worked in both builds.

Signed-off-by: Sam Day <me@samcday.com>
---
Sam Day (3):
      usb: udc: ci: support USB gadget driver model
      usb: host: ehci-msm: Register ChipIdea UDC from glue wrapper
      qcom_defconfig: Enable MSM8916 USB PHY + ChipIdea UDC

 configs/qcom_defconfig      |  6 ++++
 drivers/usb/gadget/ci_udc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/host/ehci-msm.c |  7 ++++
 3 files changed, 96 insertions(+), 1 deletion(-)
---
base-commit: a40fc5afaec079ee4e621965fed18dcc94240d8c
change-id: 20250412-msm8916-usb-a1dc93c01bd5
prerequisite-message-id: <20250407-ehci-msm-fixes-v1-0-f8b30eb05d07@linaro.org>
prerequisite-patch-id: 006e55423a2d28cc23ff11035da672b418ebec51
prerequisite-patch-id: 8c51357ca78f0465407e0a74fda666a3e540f6d8
prerequisite-patch-id: 68d707d65315e9ea7c61ba97c58257ea9a3e3152
prerequisite-patch-id: 514522633917dd1efd2380a53949c5e7ff5ff0dd
prerequisite-patch-id: 9ee7e8f41aa3070f802d087bd763ab384e681adb
prerequisite-patch-id: fbfd19c0f729fe157c31bc40d48d19d0083d23a4

Best regards,
-- 
Sam Day <me@samcday.com>

[PATCH 1/3] usb: udc: ci: support USB gadget driver model

[Sam Day]

When CONFIG_DM_USB_GADGET is enabled, a UCLASS_USB_GADGET_GENERIC driver
will be defined that wraps the ChipIdea UDC operations. The
(dm_)?usb_gadget_.* symbols will no longer be defined (as these are now
handled by the UDC uclass).

If CONFIG_DM_USB_GADGET is not enabled, this driver behaves as it
previously did.

This new driver does not declare any compatibles of its own. It requires
a glue driver to bind it. The ehci_msm driver will be updated in the
following commit to do exactly that.

Signed-off-by: Sam Day <me@samcday.com>
---
 drivers/usb/gadget/ci_udc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 4bff75da759ded6716641713fbe47f6ba79386dc..179f8540da30c693d11d772aacc00cc37f7669c5 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -10,6 +10,7 @@
 #include <command.h>
 #include <config.h>
 #include <cpu_func.h>
+#include <dm.h>
 #include <net.h>
 #include <malloc.h>
 #include <wait_bit.h>
@@ -94,8 +95,18 @@ static struct usb_request *
 ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags);
 static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req);
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+static int ci_udc_gadget_start(struct usb_gadget *g,
+		struct usb_gadget_driver *driver);
+static int ci_udc_gadget_stop(struct usb_gadget *g);
+#endif
+
 static const struct usb_gadget_ops ci_udc_ops = {
 	.pullup = ci_pullup,
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+	.udc_start = ci_udc_gadget_start,
+	.udc_stop = ci_udc_gadget_stop,
+#endif
 };
 
 static const struct usb_ep_ops ci_ep_ops = {
@@ -927,7 +938,7 @@ void udc_irq(void)
 	}
 }
 
-int dm_usb_gadget_handle_interrupts(struct udevice *dev)
+static int ci_udc_handle_interrupts(struct udevice *dev)
 {
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
 	u32 value;
@@ -1072,6 +1083,71 @@ static int ci_udc_probe(void)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+static int ci_udc_generic_probe(struct udevice *dev)
+{
+	int ret;
+#if CONFIG_IS_ENABLED(DM_USB)
+	ret = usb_setup_ehci_gadget(&controller.ctrl);
+#else
+	ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
+#endif
+	if (ret)
+		return ret;
+
+	ret = ci_udc_probe();
+	if (ret)
+		return ret;
+
+	return usb_add_gadget_udc((struct device *)dev, &controller.gadget);
+}
+
+static int ci_udc_generic_remove(struct udevice *dev)
+{
+	usb_del_gadget_udc(&controller.gadget);
+
+	if (IS_ENABLED(CONFIG_DM_USB)) {
+		usb_remove_ehci_gadget(&controller.ctrl);
+	} else {
+		usb_lowlevel_stop(0);
+		controller.ctrl = NULL;
+	}
+
+	return 0;
+}
+
+static const struct usb_gadget_generic_ops ci_udc_generic_ops = {
+	.handle_interrupts	= ci_udc_handle_interrupts,
+};
+
+U_BOOT_DRIVER(ci_udc_generic) = {
+	.name = "ci-udc",
+	.id = UCLASS_USB_GADGET_GENERIC,
+	.ops = &ci_udc_generic_ops,
+	.probe = ci_udc_generic_probe,
+	.remove = ci_udc_generic_remove,
+};
+
+static int ci_udc_gadget_start(struct usb_gadget *g,
+				 struct usb_gadget_driver *driver)
+{
+	controller.driver = driver;
+	return 0;
+}
+
+static int ci_udc_gadget_stop(struct usb_gadget *g)
+{
+	controller.driver = NULL;
+	udc_disconnect();
+
+	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
+	free(controller.items_mem);
+	free(controller.epts);
+
+	return 0;
+}
+
+#else
 int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 {
 	int ret;
@@ -1126,6 +1202,12 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	return 0;
 }
 
+int dm_usb_gadget_handle_interrupts(struct udevice *dev)
+{
+	return ci_udc_handle_interrupts(dev);
+}
+#endif
+
 bool dfu_usb_get_reset(void)
 {
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;

-- 
2.49.0

[PATCH 2/3] usb: host: ehci-msm: Register ChipIdea UDC from glue wrapper

[Sam Day]

When CONFIG_DM_USB_GADGET is enabled, the CI UDC driver needs to be
explicitly bound. So we do this from the newly introduced glue driver.

Signed-off-by: Sam Day <me@samcday.com>
---
 drivers/usb/host/ehci-msm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
index 8aeb6a915563453ad2a02721d4828fd1b300a4e4..ae40e96e6349b1a352608e6350ce950a3cff95af 100644
--- a/drivers/usb/host/ehci-msm.c
+++ b/drivers/usb/host/ehci-msm.c
@@ -161,6 +161,13 @@ static int qcom_ci_hdrc_bind(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (IS_ENABLED(CONFIG_DM_USB_GADGET) && IS_ENABLED(CONFIG_CI_UDC)) {
+		ret = device_bind_driver_to_node(dev, "ci-udc", "ci-udc",
+				dev_ofnode(dev), NULL);
+		if (ret)
+			return ret;
+	}
+
 	if (!ofnode_valid(ulpi_node))
 		return 0;
 

-- 
2.49.0

[PATCH 3/3] qcom_defconfig: Enable MSM8916 USB PHY + ChipIdea UDC

[Sam Day]

Now that the ChipIdea UDC driver supports USB Gadget driver model, we
can enable it alongside the dwc3 generic driver.

Signed-off-by: Sam Day <me@samcday.com>
---
 configs/qcom_defconfig | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
index 537806450dc4a61d3c617cdd2b0cfb8eab1c343c..e8582d4e6bd2fae4ff2f7e34dc32651ea2de76bc 100644
--- a/configs/qcom_defconfig
+++ b/configs/qcom_defconfig
@@ -45,6 +45,7 @@ CONFIG_OF_UPSTREAM_BUILD_VENDOR=y
 CONFIG_USE_DEFAULT_ENV_FILE=y
 CONFIG_DEFAULT_ENV_FILE="board/qualcomm/default.env"
 CONFIG_BUTTON_QCOM_PMIC=y
+CONFIG_CI_UDC=y
 CONFIG_CLK=y
 CONFIG_CLK_STUB=y
 CONFIG_CLK_QCOM_APQ8016=y
@@ -121,6 +122,7 @@ CONFIG_RNG_MSM=y
 CONFIG_SCSI=y
 CONFIG_MSM_SERIAL=y
 CONFIG_MSM_GENI_SERIAL=y
+CONFIG_MSM8916_USB_PHY=y
 CONFIG_SOC_QCOM=y
 CONFIG_QCOM_COMMAND_DB=y
 CONFIG_QCOM_RPMH=y
@@ -133,6 +135,10 @@ CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_XHCI_DWC3=y
 CONFIG_USB_DWC3=y
 CONFIG_USB_DWC3_GENERIC=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_EHCI_MSM=y
+CONFIG_USB_ULPI=y
+CONFIG_USB_ULPI_VIEWPORT=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_DOWNLOAD=y
 CONFIG_USB_FUNCTION_MASS_STORAGE=y

-- 
2.49.0

Re: [PATCH 1/3] usb: udc: ci: support USB gadget driver model

[Stephan Gerhold]

On Sat, Apr 12, 2025 at 07:39:57PM +0000, Sam Day wrote:
> When CONFIG_DM_USB_GADGET is enabled, a UCLASS_USB_GADGET_GENERIC driver
> will be defined that wraps the ChipIdea UDC operations. The
> (dm_)?usb_gadget_.* symbols will no longer be defined (as these are now
> handled by the UDC uclass).
> 
> If CONFIG_DM_USB_GADGET is not enabled, this driver behaves as it
> previously did.
> 
> This new driver does not declare any compatibles of its own. It requires
> a glue driver to bind it. The ehci_msm driver will be updated in the
> following commit to do exactly that.
> 
> Signed-off-by: Sam Day <me@samcday.com>

Thanks a lot for working on this. Seems to work without problems on my
dragonboard410c. Just some minor nits below to reduce code duplication.

> ---
>  drivers/usb/gadget/ci_udc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index 4bff75da759ded6716641713fbe47f6ba79386dc..179f8540da30c693d11d772aacc00cc37f7669c5 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -10,6 +10,7 @@
>  #include <command.h>
>  #include <config.h>
>  #include <cpu_func.h>
> +#include <dm.h>
>  #include <net.h>
>  #include <malloc.h>
>  #include <wait_bit.h>
> @@ -94,8 +95,18 @@ static struct usb_request *
>  ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags);
>  static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req);
>  
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +static int ci_udc_gadget_start(struct usb_gadget *g,
> +		struct usb_gadget_driver *driver);
> +static int ci_udc_gadget_stop(struct usb_gadget *g);
> +#endif
> +
>  static const struct usb_gadget_ops ci_udc_ops = {
>  	.pullup = ci_pullup,
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +	.udc_start = ci_udc_gadget_start,
> +	.udc_stop = ci_udc_gadget_stop,
> +#endif
>  };
>  
>  static const struct usb_ep_ops ci_ep_ops = {
> @@ -927,7 +938,7 @@ void udc_irq(void)
>  	}
>  }
>  
> -int dm_usb_gadget_handle_interrupts(struct udevice *dev)
> +static int ci_udc_handle_interrupts(struct udevice *dev)
>  {
>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
>  	u32 value;
> @@ -1072,6 +1083,71 @@ static int ci_udc_probe(void)
>  	return 0;
>  }
>  
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +static int ci_udc_generic_probe(struct udevice *dev)
> +{
> +	int ret;
> +#if CONFIG_IS_ENABLED(DM_USB)
> +	ret = usb_setup_ehci_gadget(&controller.ctrl);
> +#else
> +	ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
> +#endif

...

> [...]
> +static int ci_udc_generic_remove(struct udevice *dev)
> +{
> +	usb_del_gadget_udc(&controller.gadget);
> +
> +	if (IS_ENABLED(CONFIG_DM_USB)) {
> +		usb_remove_ehci_gadget(&controller.ctrl);
> +	} else {
> +		usb_lowlevel_stop(0);
> +		controller.ctrl = NULL;
> +	}

This style is nice for compile testing. But in the probe() function
above you're still using the #if/#else pre-processor style. Can it use
if (IS_ENABLED(...)) too?

	if (IS_ENABLED(CONFIG_DM_USB))
		ret = usb_setup_ehci_gadget(&controller.ctrl);
	else
		ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
	if (ret)
		return ret;

This seems to compile at least.

If yes, can you make the same cleanup for the old !DM_USB_GADGET case,
so the two code paths are consistent?

> +
> +	return 0;
> +}
> +
> +static const struct usb_gadget_generic_ops ci_udc_generic_ops = {
> +	.handle_interrupts	= ci_udc_handle_interrupts,
> +};
> +
> +U_BOOT_DRIVER(ci_udc_generic) = {
> +	.name = "ci-udc",
> +	.id = UCLASS_USB_GADGET_GENERIC,
> +	.ops = &ci_udc_generic_ops,
> +	.probe = ci_udc_generic_probe,
> +	.remove = ci_udc_generic_remove,
> +};
> +
> +static int ci_udc_gadget_start(struct usb_gadget *g,
> +				 struct usb_gadget_driver *driver)
> +{
> +	controller.driver = driver;
> +	return 0;
> +}
> +
> +static int ci_udc_gadget_stop(struct usb_gadget *g)
> +{
> +	controller.driver = NULL;
> +	udc_disconnect();
> +
> +	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
> +	free(controller.items_mem);
> +	free(controller.epts);
> +
> +	return 0;
> +}

I wonder if you could just define the start and stop function outside
the #if blocks and call them from usb_gadget_unregister_driver()
and usb_gadget_register_driver(). This would avoid duplicating the code,
reducing the risk that someone forgets to update one of those when
making changes.

The same could be also said for probe()/remove(), i.e. moving the

	if (IS_ENABLED(CONFIG_DM_USB)) {
		usb_remove_ehci_gadget(&controller.ctrl);
	} else {
		usb_lowlevel_stop(0);
		controller.ctrl = NULL;
	}

block to a separate function and calling that from both variants. Not
sure if that would also be worth it?

Thanks,
Stephan

Re: [PATCH 2/3] usb: host: ehci-msm: Register ChipIdea UDC from glue wrapper

[Stephan Gerhold]

On Sat, Apr 12, 2025 at 07:40:03PM +0000, Sam Day wrote:
> When CONFIG_DM_USB_GADGET is enabled, the CI UDC driver needs to be
> explicitly bound. So we do this from the newly introduced glue driver.
> 
> Signed-off-by: Sam Day <me@samcday.com>
> ---
>  drivers/usb/host/ehci-msm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> index 8aeb6a915563453ad2a02721d4828fd1b300a4e4..ae40e96e6349b1a352608e6350ce950a3cff95af 100644
> --- a/drivers/usb/host/ehci-msm.c
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -161,6 +161,13 @@ static int qcom_ci_hdrc_bind(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	if (IS_ENABLED(CONFIG_DM_USB_GADGET) && IS_ENABLED(CONFIG_CI_UDC)) {
> +		ret = device_bind_driver_to_node(dev, "ci-udc", "ci-udc",
> +				dev_ofnode(dev), NULL);

Nitpick: Indentation looks off here (we usually align with the open
parenthesis (

Thanks,
Stephan

Re: [PATCH 3/3] qcom_defconfig: Enable MSM8916 USB PHY + ChipIdea UDC

[Stephan Gerhold]

On Sat, Apr 12, 2025 at 07:40:11PM +0000, Sam Day wrote:
> Now that the ChipIdea UDC driver supports USB Gadget driver model, we
> can enable it alongside the dwc3 generic driver.
> 
> Signed-off-by: Sam Day <me@samcday.com>
> ---
>  configs/qcom_defconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/configs/qcom_defconfig b/configs/qcom_defconfig
> index 537806450dc4a61d3c617cdd2b0cfb8eab1c343c..e8582d4e6bd2fae4ff2f7e34dc32651ea2de76bc 100644
> --- a/configs/qcom_defconfig
> +++ b/configs/qcom_defconfig
> @@ -45,6 +45,7 @@ CONFIG_OF_UPSTREAM_BUILD_VENDOR=y
>  CONFIG_USE_DEFAULT_ENV_FILE=y
>  CONFIG_DEFAULT_ENV_FILE="board/qualcomm/default.env"
>  CONFIG_BUTTON_QCOM_PMIC=y
> +CONFIG_CI_UDC=y
>  CONFIG_CLK=y
>  CONFIG_CLK_STUB=y
>  CONFIG_CLK_QCOM_APQ8016=y
> @@ -121,6 +122,7 @@ CONFIG_RNG_MSM=y
>  CONFIG_SCSI=y
>  CONFIG_MSM_SERIAL=y
>  CONFIG_MSM_GENI_SERIAL=y
> +CONFIG_MSM8916_USB_PHY=y
>  CONFIG_SOC_QCOM=y
>  CONFIG_QCOM_COMMAND_DB=y
>  CONFIG_QCOM_RPMH=y
> @@ -133,6 +135,10 @@ CONFIG_USB_XHCI_HCD=y
>  CONFIG_USB_XHCI_DWC3=y
>  CONFIG_USB_DWC3=y
>  CONFIG_USB_DWC3_GENERIC=y
> +CONFIG_USB_EHCI_HCD=y
> +CONFIG_USB_EHCI_MSM=y
> +CONFIG_USB_ULPI=y
> +CONFIG_USB_ULPI_VIEWPORT=y
>  CONFIG_USB_GADGET=y
>  CONFIG_USB_GADGET_DOWNLOAD=y
>  CONFIG_USB_FUNCTION_MASS_STORAGE=y

Please run "make savedefconfig" "cp defconfig configs/qcom_defconfig".

Defconfigs are not alphabetically ordered, they are ordered based on the
definitions in the Kconfig files. And some of the options (e.g.
CONFIG_MSM8916_USB_PHY) will disappear because they're already selected
by other options. :-)

Thanks,
Stephan

Re: [PATCH 1/3] usb: udc: ci: support USB gadget driver model

[Sam Day]

Heya Stephan,

On Wed Apr 16, 2025 at 8:51 PM CEST, Stephan Gerhold wrote:
> On Sat, Apr 12, 2025 at 07:39:57PM +0000, Sam Day wrote:
>> When CONFIG_DM_USB_GADGET is enabled, a UCLASS_USB_GADGET_GENERIC driver
>> will be defined that wraps the ChipIdea UDC operations. The
>> (dm_)?usb_gadget_.* symbols will no longer be defined (as these are now
>> handled by the UDC uclass).
>>
>> If CONFIG_DM_USB_GADGET is not enabled, this driver behaves as it
>> previously did.
>>
>> This new driver does not declare any compatibles of its own. It requires
>> a glue driver to bind it. The ehci_msm driver will be updated in the
>> following commit to do exactly that.
>>
>> Signed-off-by: Sam Day <me@samcday.com>
>
> Thanks a lot for working on this. Seems to work without problems on my
> dragonboard410c. Just some minor nits below to reduce code duplication.

\0/ Thanks for the review and testing!

>
>> ---
>>  drivers/usb/gadget/ci_udc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>> index 4bff75da759ded6716641713fbe47f6ba79386dc..179f8540da30c693d11d772aacc00cc37f7669c5 100644
>> --- a/drivers/usb/gadget/ci_udc.c
>> +++ b/drivers/usb/gadget/ci_udc.c
>> @@ -10,6 +10,7 @@
>>  #include <command.h>
>>  #include <config.h>
>>  #include <cpu_func.h>
>> +#include <dm.h>
>>  #include <net.h>
>>  #include <malloc.h>
>>  #include <wait_bit.h>
>> @@ -94,8 +95,18 @@ static struct usb_request *
>>  ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags);
>>  static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req);
>>
>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>> +static int ci_udc_gadget_start(struct usb_gadget *g,
>> +		struct usb_gadget_driver *driver);
>> +static int ci_udc_gadget_stop(struct usb_gadget *g);
>> +#endif
>> +
>>  static const struct usb_gadget_ops ci_udc_ops = {
>>  	.pullup = ci_pullup,
>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>> +	.udc_start = ci_udc_gadget_start,
>> +	.udc_stop = ci_udc_gadget_stop,
>> +#endif
>>  };
>>
>>  static const struct usb_ep_ops ci_ep_ops = {
>> @@ -927,7 +938,7 @@ void udc_irq(void)
>>  	}
>>  }
>>
>> -int dm_usb_gadget_handle_interrupts(struct udevice *dev)
>> +static int ci_udc_handle_interrupts(struct udevice *dev)
>>  {
>>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
>>  	u32 value;
>> @@ -1072,6 +1083,71 @@ static int ci_udc_probe(void)
>>  	return 0;
>>  }
>>
>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
>> +static int ci_udc_generic_probe(struct udevice *dev)
>> +{
>> +	int ret;
>> +#if CONFIG_IS_ENABLED(DM_USB)
>> +	ret = usb_setup_ehci_gadget(&controller.ctrl);
>> +#else
>> +	ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
>> +#endif
>
> ...
>
>> [...]
>> +static int ci_udc_generic_remove(struct udevice *dev)
>> +{
>> +	usb_del_gadget_udc(&controller.gadget);
>> +
>> +	if (IS_ENABLED(CONFIG_DM_USB)) {
>> +		usb_remove_ehci_gadget(&controller.ctrl);
>> +	} else {
>> +		usb_lowlevel_stop(0);
>> +		controller.ctrl = NULL;
>> +	}
>
> This style is nice for compile testing. But in the probe() function
> above you're still using the #if/#else pre-processor style. Can it use
> if (IS_ENABLED(...)) too?

Ah, I had converted a few of these when checkpatch reprimanded me. Guess
I missed this one - oops!

>
> 	if (IS_ENABLED(CONFIG_DM_USB))
> 		ret = usb_setup_ehci_gadget(&controller.ctrl);
> 	else
> 		ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
> 	if (ret)
> 		return ret;
>
> This seems to compile at least.
>
> If yes, can you make the same cleanup for the old !DM_USB_GADGET case,
> so the two code paths are consistent?

Thanks, I've done this in v2.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct usb_gadget_generic_ops ci_udc_generic_ops = {
>> +	.handle_interrupts	= ci_udc_handle_interrupts,
>> +};
>> +
>> +U_BOOT_DRIVER(ci_udc_generic) = {
>> +	.name = "ci-udc",
>> +	.id = UCLASS_USB_GADGET_GENERIC,
>> +	.ops = &ci_udc_generic_ops,
>> +	.probe = ci_udc_generic_probe,
>> +	.remove = ci_udc_generic_remove,
>> +};
>> +
>> +static int ci_udc_gadget_start(struct usb_gadget *g,
>> +				 struct usb_gadget_driver *driver)
>> +{
>> +	controller.driver = driver;
>> +	return 0;
>> +}
>> +
>> +static int ci_udc_gadget_stop(struct usb_gadget *g)
>> +{
>> +	controller.driver = NULL;
>> +	udc_disconnect();
>> +
>> +	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
>> +	free(controller.items_mem);
>> +	free(controller.epts);
>> +
>> +	return 0;
>> +}
>
> I wonder if you could just define the start and stop function outside
> the #if blocks and call them from usb_gadget_unregister_driver()
> and usb_gadget_register_driver(). This would avoid duplicating the code,
> reducing the risk that someone forgets to update one of those when
> making changes.

So, not quite unfortunately. In the case of ci_udc_gadget_stop(), the
controller.driver = NULL happening before calling into udc_disconnect()
is important: the generic UDC uclass code has already called
driver->disconnect() and calling it again is a Very Bad Thing to do. So
we can't just delegate usb_gadget_unregister_driver to
ci_udc_gadget_stop outright.

In v2 I've tried to unify the codepaths as much as possible though,
both to avoid duplication and the risks of future patches making
incomplete changes.

>
> The same could be also said for probe()/remove(), i.e. moving the
>
> 	if (IS_ENABLED(CONFIG_DM_USB)) {
> 		usb_remove_ehci_gadget(&controller.ctrl);
> 	} else {
> 		usb_lowlevel_stop(0);
> 		controller.ctrl = NULL;
> 	}
>
> block to a separate function and calling that from both variants. Not
> sure if that would also be worth it?

Good call, these ones are less problematic, and I've done that in v2.

Rock on \m/,
-Sam

>
> Thanks,
> Stephan

Re: [PATCH 1/3] usb: udc: ci: support USB gadget driver model

[Wojciech Dubowik]

On Sat, Apr 19, 2025 at 09:32:04AM +0000, Sam Day wrote:
> Heya Stephan,
> 
> On Wed Apr 16, 2025 at 8:51 PM CEST, Stephan Gerhold wrote:
> > On Sat, Apr 12, 2025 at 07:39:57PM +0000, Sam Day wrote:
> >> When CONFIG_DM_USB_GADGET is enabled, a UCLASS_USB_GADGET_GENERIC driver
> >> will be defined that wraps the ChipIdea UDC operations. The
> >> (dm_)?usb_gadget_.* symbols will no longer be defined (as these are now
> >> handled by the UDC uclass).
> >>
> >> If CONFIG_DM_USB_GADGET is not enabled, this driver behaves as it
> >> previously did.
> >>
> >> This new driver does not declare any compatibles of its own. It requires
> >> a glue driver to bind it. The ehci_msm driver will be updated in the
> >> following commit to do exactly that.
> >>
> >> Signed-off-by: Sam Day <me at samcday.com>
> >
> > Thanks a lot for working on this. Seems to work without problems on my
> > dragonboard410c. Just some minor nits below to reduce code duplication.
> 
> \0/ Thanks for the review and testing!
> 
> >
> >> ---
> >>  drivers/usb/gadget/ci_udc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 83 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> >> index 4bff75da759ded6716641713fbe47f6ba79386dc..179f8540da30c693d11d772aacc00cc37f7669c5 100644
> >> --- a/drivers/usb/gadget/ci_udc.c
> >> +++ b/drivers/usb/gadget/ci_udc.c
> >> @@ -10,6 +10,7 @@
> >>  #include <command.h>
> >>  #include <config.h>
> >>  #include <cpu_func.h>
> >> +#include <dm.h>
> >>  #include <net.h>
> >>  #include <malloc.h>
> >>  #include <wait_bit.h>
> >> @@ -94,8 +95,18 @@ static struct usb_request *
> >>  ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags);
> >>  static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req);
> >>
> >> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> >> +static int ci_udc_gadget_start(struct usb_gadget *g,
> >> +		struct usb_gadget_driver *driver);
> >> +static int ci_udc_gadget_stop(struct usb_gadget *g);
> >> +#endif
> >> +
> >>  static const struct usb_gadget_ops ci_udc_ops = {
> >>  	.pullup = ci_pullup,
> >> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> >> +	.udc_start = ci_udc_gadget_start,
> >> +	.udc_stop = ci_udc_gadget_stop,
> >> +#endif
> >>  };
> >>
> >>  static const struct usb_ep_ops ci_ep_ops = {
> >> @@ -927,7 +938,7 @@ void udc_irq(void)
> >>  	}
> >>  }
> >>
> >> -int dm_usb_gadget_handle_interrupts(struct udevice *dev)
> >> +static int ci_udc_handle_interrupts(struct udevice *dev)
> >>  {
> >>  	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
> >>  	u32 value;
> >> @@ -1072,6 +1083,71 @@ static int ci_udc_probe(void)
> >>  	return 0;
> >>  }
> >>
> >> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> >> +static int ci_udc_generic_probe(struct udevice *dev)
> >> +{
> >> +	int ret;
> >> +#if CONFIG_IS_ENABLED(DM_USB)
> >> +	ret = usb_setup_ehci_gadget(&controller.ctrl);
> >> +#else
> >> +	ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
> >> +#endif
> >
> > ...
> >
> >> [...]
> >> +static int ci_udc_generic_remove(struct udevice *dev)
> >> +{
> >> +	usb_del_gadget_udc(&controller.gadget);
> >> +
> >> +	if (IS_ENABLED(CONFIG_DM_USB)) {
> >> +		usb_remove_ehci_gadget(&controller.ctrl);
> >> +	} else {
> >> +		usb_lowlevel_stop(0);
> >> +		controller.ctrl = NULL;
> >> +	}
> >
> > This style is nice for compile testing. But in the probe() function
> > above you're still using the #if/#else pre-processor style. Can it use
> > if (IS_ENABLED(...)) too?
> 
> Ah, I had converted a few of these when checkpatch reprimanded me. Guess
> I missed this one - oops!
> 
> >
> > 	if (IS_ENABLED(CONFIG_DM_USB))
> > 		ret = usb_setup_ehci_gadget(&controller.ctrl);
> > 	else
> > 		ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl);
> > 	if (ret)
> > 		return ret;
> >
> > This seems to compile at least.
> >
> > If yes, can you make the same cleanup for the old !DM_USB_GADGET case,
> > so the two code paths are consistent?
> 
> Thanks, I've done this in v2.
> 
> >
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct usb_gadget_generic_ops ci_udc_generic_ops = {
> >> +	.handle_interrupts	= ci_udc_handle_interrupts,
> >> +};
> >> +
> >> +U_BOOT_DRIVER(ci_udc_generic) = {
> >> +	.name = "ci-udc",
> >> +	.id = UCLASS_USB_GADGET_GENERIC,
> >> +	.ops = &ci_udc_generic_ops,
> >> +	.probe = ci_udc_generic_probe,
> >> +	.remove = ci_udc_generic_remove,
> >> +};
> >> +
> >> +static int ci_udc_gadget_start(struct usb_gadget *g,
> >> +				 struct usb_gadget_driver *driver)
> >> +{
> >> +	controller.driver = driver;
> >> +	return 0;
> >> +}
> >> +
> >> +static int ci_udc_gadget_stop(struct usb_gadget *g)
> >> +{
> >> +	controller.driver = NULL;
> >> +	udc_disconnect();
> >> +
> >> +	ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
> >> +	free(controller.items_mem);
> >> +	free(controller.epts);
> >> +
> >> +	return 0;
> >> +}
> >
> > I wonder if you could just define the start and stop function outside
> > the #if blocks and call them from usb_gadget_unregister_driver()
> > and usb_gadget_register_driver(). This would avoid duplicating the code,
> > reducing the risk that someone forgets to update one of those when
> > making changes.
> 
> So, not quite unfortunately. In the case of ci_udc_gadget_stop(), the
> controller.driver = NULL happening before calling into udc_disconnect()
> is important: the generic UDC uclass code has already called
> driver->disconnect() and calling it again is a Very Bad Thing to do. So
> we can't just delegate usb_gadget_unregister_driver to
> ci_udc_gadget_stop outright.
> 
> In v2 I've tried to unify the codepaths as much as possible though,
> both to avoid duplication and the risks of future patches making
> incomplete changes.
> 
> >
> > The same could be also said for probe()/remove(), i.e. moving the
> >
> > 	if (IS_ENABLED(CONFIG_DM_USB)) {
> > 		usb_remove_ehci_gadget(&controller.ctrl);
> > 	} else {
> > 		usb_lowlevel_stop(0);
> > 		controller.ctrl = NULL;
> > 	}
> >
> > block to a separate function and calling that from both variants. Not
> > sure if that would also be worth it?
> 
> Good call, these ones are less problematic, and I've done that in v2.
Hello Sam,

Are you going to send v2 soon? I am just asking as I had something similar
in the pipeline.

Regrads,
Wojtek

> 
> Rock on \m/,
> -Sam
> 
> >
> > Thanks,
> > Stephan
> 
>