[PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[Sam Edwards]

Happy Thursday, U-Boot list!

Here is attempt 2 at making this USB controller driver compatible with
DM's gadget model, following what most of the other musb variants do.

v2 removes the unwanted printfs in the probe func, per feedback from Marek.
I received no other feedback against v1 of this patch.

Cheers,
Sam

Sam Edwards (2):
  usb: musb-new: sunxi: remove unwanted printfs
  usb: musb-new: sunxi: make compatible with UDC/DM gadget model

 drivers/usb/musb-new/sunxi.c | 52 +++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] usb: musb-new: sunxi: remove unwanted printfs

[Sam Edwards]

Per Marek's feedback, unconditional printfs in the probe function of this
USB controller driver should be removed.

This patch also slightly tidies up the return path, in preparation for
DM support.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/usb/musb-new/sunxi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index c1c0087f7d..510b254f7d 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -494,19 +494,15 @@ static int musb_usb_probe(struct udevice *dev)
 	if (!host->host)
 		return -EIO;
 
-	ret = musb_lowlevel_init(host);
-	if (!ret)
-		printf("Allwinner mUSB OTG (Host)\n");
+	return musb_lowlevel_init(host);
 #else
 	pdata.mode = MUSB_PERIPHERAL;
 	host->host = musb_register(&pdata, &glue->dev, base);
 	if (IS_ERR_OR_NULL(host->host))
 		return -EIO;
 
-	printf("Allwinner mUSB OTG (Peripheral)\n");
+	return 0;
 #endif
-
-	return ret;
 }
 
 static int musb_usb_remove(struct udevice *dev)
-- 
2.39.2

[PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Sam Edwards]

Since many sunxi boards do not implement a `board_usb_init`, it's
better if we just make the sunxi USB driver compatible with the
DM gadget model, as many other musb-new variants already are.

This change has been verified working on a T113s.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/usb/musb-new/sunxi.c | 50 +++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 510b254f7d..6658cd995d 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -444,6 +444,16 @@ static struct musb_hdrc_config musb_config_h3 = {
 	.ram_bits	= SUNXI_MUSB_RAM_BITS,
 };
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+int dm_usb_gadget_handle_interrupts(struct udevice *dev) {
+	struct sunxi_glue *glue = dev_get_priv(dev);
+	struct musb_host_data *host = &glue->mdata;
+
+	host->host->isr(0, host->host);
+	return 0;
+}
+#endif
+
 static int musb_usb_probe(struct udevice *dev)
 {
 	struct sunxi_glue *glue = dev_get_priv(dev);
@@ -452,10 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
 	void *base = dev_read_addr_ptr(dev);
 	int ret;
 
-#ifdef CONFIG_USB_MUSB_HOST
-	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
-#endif
-
 	if (!base)
 		return -EINVAL;
 
@@ -486,23 +492,31 @@ static int musb_usb_probe(struct udevice *dev)
 	pdata.platform_ops = &sunxi_musb_ops;
 	pdata.config = glue->cfg->config;
 
-#ifdef CONFIG_USB_MUSB_HOST
-	priv->desc_before_addr = true;
+	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
+		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
+		priv->desc_before_addr = true;
 
-	pdata.mode = MUSB_HOST;
-	host->host = musb_init_controller(&pdata, &glue->dev, base);
-	if (!host->host)
-		return -EIO;
+		pdata.mode = MUSB_HOST;
+		host->host = musb_init_controller(&pdata, &glue->dev, base);
+		if (!host->host)
+			return -EIO;
 
-	return musb_lowlevel_init(host);
-#else
-	pdata.mode = MUSB_PERIPHERAL;
-	host->host = musb_register(&pdata, &glue->dev, base);
-	if (IS_ERR_OR_NULL(host->host))
-		return -EIO;
+		return musb_lowlevel_init(host);
+	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
+		pdata.mode = MUSB_PERIPHERAL;
+		host->host = musb_init_controller(&pdata, &glue->dev, base);
+		if (!host->host)
+			return -EIO;
 
-	return 0;
-#endif
+		return usb_add_gadget_udc(&glue->dev, &host->host->g);
+	} else {
+		pdata.mode = MUSB_PERIPHERAL;
+		host->host = musb_register(&pdata, &glue->dev, base);
+		if (IS_ERR_OR_NULL(host->host))
+			return -EIO;
+
+		return 0;
+	}
 }
 
 static int musb_usb_remove(struct udevice *dev)
-- 
2.39.2

Re: [PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[John Watts]

Hi there,

I've tested this patch and it seems to support the gadget model, but I'm having
a lot of USB errors. What device did you test this on?

John.

On Thu, Jun 08, 2023 at 01:56:29PM -0600, Sam Edwards wrote:
> Happy Thursday, U-Boot list!
> 
> Here is attempt 2 at making this USB controller driver compatible with
> DM's gadget model, following what most of the other musb variants do.
> 
> v2 removes the unwanted printfs in the probe func, per feedback from Marek.
> I received no other feedback against v1 of this patch.
> 
> Cheers,
> Sam
> 
> Sam Edwards (2):
>   usb: musb-new: sunxi: remove unwanted printfs
>   usb: musb-new: sunxi: make compatible with UDC/DM gadget model
> 
>  drivers/usb/musb-new/sunxi.c | 52 +++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[Sam Edwards]

On Thu, Apr 11, 2024 at 1:40 AM John Watts <contact@jookia.org> wrote:
>
> On Thu, Apr 11, 2024 at 12:52:14AM -0600, Sam Edwards wrote:
> > Hi John,
> >
> > This patch was developed against (and used very heavily on) the Turing
> > Pi 2, which has an Allwinner T113-s3 SoC. Likely it should work for
> > any T113/D1 board. I haven't been encountering any USB errors but also
> > my use case hasn't gone much beyond the `udc` command. What
> > device/errors do you have over there?
> >
> > Cheers,
> > Sam
>
> Hi Sam,
>
> I made a list of things that do work:
>
> - DFU (slowly, probably due to no DMA) to RAM
> - CDC serial console
>
> Running a command like this:
>
> ubi part ubi; ubifsmount ubi:root; ums 0 ubi 0;

Hi John,

Ahh I see the problem. In U-Boot, `ubi` isn't actually a block device:
it's implemented as a stub in the block layer, and the filesystem
layer redirects `ubi` accesses to the currently-mounted ubifs instead.
But the `ums` command works on the block layer, so it's not being
intercepted, and instead hitting that stub and likely crashing. In the
spirit of the Linux ubiblock driver, I have another patchset[1] I've
been working on[2] to expose static ubivols as true read-only block
devices. Note that this only works for static volumes: the access
semantics of dynamic volumes are too flash-like to support block
device access, so accessing them with `ums` likely will never work
like users expect.

Still, there could be some interaction issue between `ums`<->USB that
I haven't identified. Could you try with mmc (if available) or a
ramdisk (otherwise) just to confirm that `ums` is fine?

Regards,
Sam

[1] https://lore.kernel.org/u-boot/20230812000606.72319-1-CFSworks@gmail.com/T/
[2] Not very diligently; if you're interested in helping test it, I'd
love to get back to it.

>
> Gives this output in dmesg:
>
> [3633079.772330] usb-storage 1-1.1:1.0: USB Mass Storage device detected
> [3633079.772506] scsi host9: usb-storage 1-1.1:1.0
> [3633080.794607] scsi 9:0:0:0: Direct-Access     Linux    UMS disk 0       ffff PQ: 0 ANSI: 2
> [3633080.794941] sd 9:0:0:0: Attached scsi generic sg6 type 0
> [3633080.795214] sd 9:0:0:0: [sdg] 3942645758 512-byte logical blocks: (2.02 TB/1.83 TiB)
> [3633080.795220] sd 9:0:0:0: [sdg] 3925868545-byte physical blocks
> [3633080.795341] sd 9:0:0:0: [sdg] Write Protect is off
> [3633080.795345] sd 9:0:0:0: [sdg] Mode Sense: 0f 00 00 00
> [3633080.795462] sd 9:0:0:0: [sdg] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
> [3633080.907359] usb 1-1.1: reset high-speed USB device number 44 using xhci_hcd
> [3633081.021905] usb 1-1.1: device descriptor read/64, error -71
> [3633081.238566] usb 1-1.1: device descriptor read/64, error -71
> [3633081.448573] usb 1-1.1: reset high-speed USB device number 44 using xhci_hcd
> [3633081.558566] usb 1-1.1: device descriptor read/64, error -71
> [3633081.775236] usb 1-1.1: device descriptor read/64, error -71
> [3633081.988559] usb 1-1.1: reset high-speed USB device number 44 using xhci_hcd
> [3633086.788615] usb 1-1.1: Device not responding to setup address.
> [3633091.799190] usb 1-1.1: Device not responding to setup address.
> [3633092.008482] usb 1-1.1: device not accepting address 44, error -71
> [3633092.747719] usb 1-1.1: USB disconnect, device number 44
> [3633092.748488] device offline error, dev sdg, sector 0 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748502] buffer_io_error: 10 callbacks suppressed
> [3633092.748504] Buffer I/O error on dev sdg, logical block 0, async page read
> [3633092.748511] Buffer I/O error on dev sdg, logical block 1, async page read
> [3633092.748520] device offline error, dev sdg, sector 4 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748525] Buffer I/O error on dev sdg, logical block 2, async page read
> [3633092.748529] Buffer I/O error on dev sdg, logical block 3, async page read
> [3633092.748582] device offline error, dev sdg, sector 0 op 0x0:(READ) flags 0x0 phys_seg 4 prio class 2
> [3633092.748594] Buffer I/O error on dev sdg, logical block 0, async page read
> [3633092.748600] Buffer I/O error on dev sdg, logical block 1, async page read
> [3633092.748605] Buffer I/O error on dev sdg, logical block 2, async page read
> [3633092.748609] Buffer I/O error on dev sdg, logical block 3, async page read
> [3633092.748621] ldm_validate_partition_table(): Disk read failed.
> [3633092.748638] device offline error, dev sdg, sector 0 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748644] Buffer I/O error on dev sdg, logical block 0, async page read
> [3633092.748649] Buffer I/O error on dev sdg, logical block 1, async page read
> [3633092.748665] device offline error, dev sdg, sector 4 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748686] device offline error, dev sdg, sector 0 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748704] device offline error, dev sdg, sector 4 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748726] device offline error, dev sdg, sector 0 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748744] device offline error, dev sdg, sector 4 op 0x0:(READ) flags 0x0 phys_seg 2 prio class 2
> [3633092.748754]  sdg: unable to read partition table
> [3633092.748825] sd 9:0:0:0: [sdg] Attached SCSI removable disk
>
> Hitting Ctrl-C in U-Boot then re-running the command gives this output:
>
> CTRL+C - Operation aborted
> => ubi part ubi; ubifsmount ubi:root; ums 0 ubi 0;
> UBI partition 'ubi' already selected
> UMS: LUN 0, dev ubi -352321538, hwpart 234, sector 0x0, count 0xeafffffe
> prefetch abort
> pc : [<e8fd9ffa>]          lr : [<45fb7665>]
> reloc pc : [<e5e53ffa>]    lr : [<42e31665>]
> sp : 45d5f970  ip : 45dd66a8     fp : 00000000
> r10: 45ff88ec  r9 : 45d65eb0     r8 : 45dd1fe8
> r7 : 45dd1fd8  r6 : 45ff5f18     r5 : 45dd5600  r4 : 45fff4f0
> r3 : e8fd9fff  r2 : 45ff3260     r1 : 00000005  r0 : 45dd5594
> Flags: Nzcv  IRQs off  FIQs off  Mode SVC_32 (T)
> Code: ffff ffff fefd ffff (ffff) ffff
> Resetting CPU ...
>
> Fastboot instantly downloads an image but produces this output:
>
> => fastboot 0
> musb-hdrc: peripheral reset irq lost!
> Starting download of 7317504 bytes
> .......................................................
> downloading of 7317504 bytes finished
> Booting kernel at 42000000...
>
>
> Wrong Image Type for bootm command
> ERROR -91: can't get kernel image!
> resetting ...
>
>
> Running 'fastboot stage' to just upload an image then hitting Ctrl-C in U-Boot gives this error:
>
> undefined instruction
> pc : [<45d69280>]          lr : [<45fb8c0b>]
> reloc pc : [<42be3280>]    lr : [<42e32c0b>]
> sp : 45d5fa28  ip : 45d6b59c     fp : 00000000
> r10: 00000002  r9 : 45d65eb0     r8 : 00000000
> r7 : 00000002  r6 : 45d68c3c     r5 : 00000000  r4 : 45d69288
> r3 : 45d69280  r2 : 00000001     r1 : 00001238  r0 : 45d69288
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
> Code: ffffffff ffffffff ffffffff ffffffff (ffffffff)
> Resetting CPU ...
>
> Not too sure what's happening here.
>
> John.
>
> >
> > >
> > > John.
> > >
> > > On Thu, Jun 08, 2023 at 01:56:29PM -0600, Sam Edwards wrote:
> > > > Happy Thursday, U-Boot list!
> > > >
> > > > Here is attempt 2 at making this USB controller driver compatible with
> > > > DM's gadget model, following what most of the other musb variants do.
> > > >
> > > > v2 removes the unwanted printfs in the probe func, per feedback from Marek.
> > > > I received no other feedback against v1 of this patch.
> > > >
> > > > Cheers,
> > > > Sam
> > > >
> > > > Sam Edwards (2):
> > > >   usb: musb-new: sunxi: remove unwanted printfs
> > > >   usb: musb-new: sunxi: make compatible with UDC/DM gadget model
> > > >
> > > >  drivers/usb/musb-new/sunxi.c | 52 +++++++++++++++++++++---------------
> > > >  1 file changed, 31 insertions(+), 21 deletions(-)
> > > >
> > > > --
> > > > 2.39.2
> > > >

Re: [PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[John Watts]

On Thu, Apr 11, 2024 at 03:53:51PM -0600, Sam Edwards wrote:
> Hi John,

Hi Sam,

> Ahh I see the problem. In U-Boot, `ubi` isn't actually a block device:
> it's implemented as a stub in the block layer, and the filesystem
> layer redirects `ubi` accesses to the currently-mounted ubifs instead.
> But the `ums` command works on the block layer, so it's not being
> intercepted, and instead hitting that stub and likely crashing. In the
> spirit of the Linux ubiblock driver, I have another patchset[1] I've
> been working on[2] to expose static ubivols as true read-only block
> devices. Note that this only works for static volumes: the access
> semantics of dynamic volumes are too flash-like to support block
> device access, so accessing them with `ums` likely will never work
> like users expect.

I'll give a patchset a test when I can.

> Still, there could be some interaction issue between `ums`<->USB that
> I haven't identified. Could you try with mmc (if available) or a
> ramdisk (otherwise) just to confirm that `ums` is fine?

I'll try testing with those if possible.

I did find out that fastboot actually works, and is much, much faster than DFU
for some reason.

John.

> 
> Regards,
> Sam
> 
> [1] https://lore.kernel.org/u-boot/20230812000606.72319-1-CFSworks@gmail.com/T/
> [2] Not very diligently; if you're interested in helping test it, I'd
> love to get back to it.

Re: [PATCH v2 0/2] sunxi, usb: UDC/DM gadget model support

[John Watts]

On Thu, Jun 08, 2023 at 01:56:29PM -0600, Sam Edwards wrote:
> Happy Thursday, U-Boot list!
> 
> Here is attempt 2 at making this USB controller driver compatible with
> DM's gadget model, following what most of the other musb variants do.
> 
> v2 removes the unwanted printfs in the probe func, per feedback from Marek.
> I received no other feedback against v1 of this patch.

Hi Sam,

I did some more testing and I believe my USB issues mentioned in the
other subthread are unrelated to this patch.

As such, here's my reviewed-by and tested-by. :)

John.

Reviewed-by: John Watts <contact@jookia.org>
Tested-by: John Watts <contact@jookia.org>

> 
> Cheers,
> Sam
> 
> Sam Edwards (2):
>   usb: musb-new: sunxi: remove unwanted printfs
>   usb: musb-new: sunxi: make compatible with UDC/DM gadget model
> 
>  drivers/usb/musb-new/sunxi.c | 52 +++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Andre Przywara]

On Thu,  8 Jun 2023 13:56:31 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi,

John asked me have a look at this.

> Since many sunxi boards do not implement a `board_usb_init`, it's

I am confused, what has this have to do with gadget support? *No* sunxi
board build provides board_usb_init(), but apparently this works fine for
now.
I am all for this converting to DM part, but the rationale seems a bit
off.

Also can you give some reason for this patch? What does this fix or
improve? "it's better" is a bit thin, "complying with DM" would already be
sufficient, but maybe there is more?

> better if we just make the sunxi USB driver compatible with the
> DM gadget model, as many other musb-new variants already are.
> 
> This change has been verified working on a T113s.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 50 +++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 510b254f7d..6658cd995d 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -444,6 +444,16 @@ static struct musb_hdrc_config musb_config_h3 = {
>  	.ram_bits	= SUNXI_MUSB_RAM_BITS,
>  };
>  
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)

Please no more #ifdef's. Is there any reason to *not* force
DM_USB_GADGET now, for all sunxi boards, in Kconfig?
Either by "select"ing it in the USB Kconfig, or in arch/arm/Kconfig, like
other platforms do.
Then you don't need to care about the !DM_USB_GADGET definition of this
function and can drop the #ifdef.

> +int dm_usb_gadget_handle_interrupts(struct udevice *dev) {

coding style

> +	struct sunxi_glue *glue = dev_get_priv(dev);
> +	struct musb_host_data *host = &glue->mdata;
> +
> +	host->host->isr(0, host->host);
> +	return 0;
> +}
> +#endif
> +
>  static int musb_usb_probe(struct udevice *dev)
>  {
>  	struct sunxi_glue *glue = dev_get_priv(dev);
> @@ -452,10 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>  	void *base = dev_read_addr_ptr(dev);
>  	int ret;
>  
> -#ifdef CONFIG_USB_MUSB_HOST
> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> -#endif
> -
>  	if (!base)
>  		return -EINVAL;
>  
> @@ -486,23 +492,31 @@ static int musb_usb_probe(struct udevice *dev)
>  	pdata.platform_ops = &sunxi_musb_ops;
>  	pdata.config = glue->cfg->config;
>  
> -#ifdef CONFIG_USB_MUSB_HOST
> -	priv->desc_before_addr = true;
> +	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
> +		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> +		priv->desc_before_addr = true;
>  
> -	pdata.mode = MUSB_HOST;
> -	host->host = musb_init_controller(&pdata, &glue->dev, base);
> -	if (!host->host)
> -		return -EIO;
> +		pdata.mode = MUSB_HOST;
> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
> +		if (!host->host)
> +			return -EIO;
>  
> -	return musb_lowlevel_init(host);
> -#else
> -	pdata.mode = MUSB_PERIPHERAL;
> -	host->host = musb_register(&pdata, &glue->dev, base);
> -	if (IS_ERR_OR_NULL(host->host))
> -		return -EIO;
> +		return musb_lowlevel_init(host);
> +	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
> +		pdata.mode = MUSB_PERIPHERAL;
> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
> +		if (!host->host)
> +			return -EIO;
>  
> -	return 0;
> -#endif
> +		return usb_add_gadget_udc(&glue->dev, &host->host->g);
> +	} else {
> +		pdata.mode = MUSB_PERIPHERAL;
> +		host->host = musb_register(&pdata, &glue->dev, base);
> +		if (IS_ERR_OR_NULL(host->host))
> +			return -EIO;
> +
> +		return 0;
> +	}

That looks like a good cleanup! Just need to test it briefly, but it seems
like the gist of this patch is fine.

Cheers,
Andre


>  }
>  
>  static int musb_usb_remove(struct udevice *dev)

Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Sam Edwards]

On 6/27/24 09:06, Andre Przywara wrote:
> On Thu,  8 Jun 2023 13:56:31 -0600
> Sam Edwards <cfsworks@gmail.com> wrote:
> 
> Hi,
> 
> John asked me have a look at this.

Hi Andre, it's good to hear from you again,

I'd first like to make sure you're aware that the date on this patch is 
June *2023,* not June 2024. It's possible things have changed 
substantially in the past year. I do not know if this patch is still a 
necessity; though if John is nudging about it, it probably is.

> 
>> Since many sunxi boards do not implement a `board_usb_init`, it's
> 
> I am confused, what has this have to do with gadget support? *No* sunxi
> board build provides board_usb_init(), but apparently this works fine for
> now.
> I am all for this converting to DM part, but the rationale seems a bit
> off.

For context, board_usb_init() is (was?) the non-DM entry point for USB 
functionality; it is (was?) *the* implementation of 
usb_gadget_initialize() when !DM_USB_GADGET.

> 
> Also can you give some reason for this patch? What does this fix or
> improve? "it's better" is a bit thin, "complying with DM" would already be
> sufficient, but maybe there is more?

Eh, yeah, "better" is something of a question-begging word isn't it? :)

The main point is to be compatible with DM's view of UDC, which as you 
said is a worthy goal in itself. It's "better" because this allows using 
DM's all-purpose implementation of usb_gadget_initialize(), which is 
(was?) necessary for those targets lacking board_usb_init().

> 
>> better if we just make the sunxi USB driver compatible with the
>> DM gadget model, as many other musb-new variants already are.
>>
>> This change has been verified working on a T113s.
>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
>> ---
>>   drivers/usb/musb-new/sunxi.c | 50 +++++++++++++++++++++++-------------
>>   1 file changed, 32 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
>> index 510b254f7d..6658cd995d 100644
>> --- a/drivers/usb/musb-new/sunxi.c
>> +++ b/drivers/usb/musb-new/sunxi.c
>> @@ -444,6 +444,16 @@ static struct musb_hdrc_config musb_config_h3 = {
>>   	.ram_bits	= SUNXI_MUSB_RAM_BITS,
>>   };
>>   
>> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> 
> Please no more #ifdef's. Is there any reason to *not* force
> DM_USB_GADGET now, for all sunxi boards, in Kconfig?
> Either by "select"ing it in the USB Kconfig, or in arch/arm/Kconfig, like
> other platforms do.
> Then you don't need to care about the !DM_USB_GADGET definition of this
> function and can drop the #ifdef.

I wouldn't be the one to ask. I can't think of any such reason myself. 
But to me it sounds like since *no sunxi board provides 
board_usb_init()* the only way USB gadgets *could* work is with 
DM_USB_GADGET? That'd be reason enough to force it.

> 
>> +int dm_usb_gadget_handle_interrupts(struct udevice *dev) {
> 
> coding style

Sentence fragments are harder to understand. I am assuming you are 
saying, "Please put the opening '{' on its own line."

> 
>> +	struct sunxi_glue *glue = dev_get_priv(dev);
>> +	struct musb_host_data *host = &glue->mdata;
>> +
>> +	host->host->isr(0, host->host);
>> +	return 0;
>> +}
>> +#endif
>> +
>>   static int musb_usb_probe(struct udevice *dev)
>>   {
>>   	struct sunxi_glue *glue = dev_get_priv(dev);
>> @@ -452,10 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
>>   	void *base = dev_read_addr_ptr(dev);
>>   	int ret;
>>   
>> -#ifdef CONFIG_USB_MUSB_HOST
>> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> -#endif
>> -
>>   	if (!base)
>>   		return -EINVAL;
>>   
>> @@ -486,23 +492,31 @@ static int musb_usb_probe(struct udevice *dev)
>>   	pdata.platform_ops = &sunxi_musb_ops;
>>   	pdata.config = glue->cfg->config;
>>   
>> -#ifdef CONFIG_USB_MUSB_HOST
>> -	priv->desc_before_addr = true;
>> +	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
>> +		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
>> +		priv->desc_before_addr = true;
>>   
>> -	pdata.mode = MUSB_HOST;
>> -	host->host = musb_init_controller(&pdata, &glue->dev, base);
>> -	if (!host->host)
>> -		return -EIO;
>> +		pdata.mode = MUSB_HOST;
>> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
>> +		if (!host->host)
>> +			return -EIO;
>>   
>> -	return musb_lowlevel_init(host);
>> -#else
>> -	pdata.mode = MUSB_PERIPHERAL;
>> -	host->host = musb_register(&pdata, &glue->dev, base);
>> -	if (IS_ERR_OR_NULL(host->host))
>> -		return -EIO;
>> +		return musb_lowlevel_init(host);
>> +	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
>> +		pdata.mode = MUSB_PERIPHERAL;
>> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
>> +		if (!host->host)
>> +			return -EIO;
>>   
>> -	return 0;
>> -#endif
>> +		return usb_add_gadget_udc(&glue->dev, &host->host->g);
>> +	} else {
>> +		pdata.mode = MUSB_PERIPHERAL;
>> +		host->host = musb_register(&pdata, &glue->dev, base);
>> +		if (IS_ERR_OR_NULL(host->host))
>> +			return -EIO;
>> +
>> +		return 0;
>> +	}
> 
> That looks like a good cleanup! Just need to test it briefly, but it seems
> like the gist of this patch is fine.

I think it would be wise to test it a little better than "briefly" given 
the age of the patch. I'm not well-equipped to do any testing myself 
right now or I'd volunteer.

> 
> Cheers,
> Andre

Likewise,
Sam

> 
> 
>>   }
>>   
>>   static int musb_usb_remove(struct udevice *dev)
> 

Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Andre Przywara]

On Thu, 27 Jun 2024 17:25:57 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

thanks for coming back so quickly!

> On 6/27/24 09:06, Andre Przywara wrote:
> > On Thu,  8 Jun 2023 13:56:31 -0600
> > Sam Edwards <cfsworks@gmail.com> wrote:
> > 
> > Hi,
> > 
> > John asked me have a look at this.  
> 
> Hi Andre, it's good to hear from you again,
> 
> I'd first like to make sure you're aware that the date on this patch is 
> June *2023,* not June 2024. It's possible things have changed 
> substantially in the past year.

Not in the Allwinner MUSB driver, but indeed the gadget support in U-Boot
has changed since then.

> I do not know if this patch is still a 
> necessity; though if John is nudging about it, it probably is.

Yes apparently he needs it, though I am not entirely sure why.
USB gadget has worked for ages in sunxi, without DM_USB_GADGET support,
that's why I was a bit puzzled why this patch seems so important.

And secondly I was put off by John's initial reply that it would trigger
many USB errors for him. He later rectified it, but I must have missed
that message.

> >> Since many sunxi boards do not implement a `board_usb_init`, it's  
> > 
> > I am confused, what has this have to do with gadget support? *No* sunxi
> > board build provides board_usb_init(), but apparently this works fine for
> > now.
> > I am all for this converting to DM part, but the rationale seems a bit
> > off.  
> 
> For context, board_usb_init() is (was?) the non-DM entry point for USB 
> functionality;

I think board_usb_init() comes from the old days, to provide hooks in case
certain boards need something special to enable USB, like an extra
regulator, power domain or clock to toggle. These days this should all be
covered by DT properties, so we should not (and do not!) need it.
And there is an empty fallback implementation in common/usb.c.

> it is (was?) *the* implementation of 
> usb_gadget_initialize() when !DM_USB_GADGET.

Mmh, was that so? None of them seemed to call usb_gadget_initialize(),
though? I checked older releases, but usb_gadget_initialize() seemed to
have always been called by the respective gadget code (fastboot, ums,
ethernet)?

> > Also can you give some reason for this patch? What does this fix or
> > improve? "it's better" is a bit thin, "complying with DM" would already be
> > sufficient, but maybe there is more?  
> 
> Eh, yeah, "better" is something of a question-begging word isn't it? :)
> 
> The main point is to be compatible with DM's view of UDC, which as you 
> said is a worthy goal in itself. It's "better" because this allows using 
> DM's all-purpose implementation of usb_gadget_initialize(), which is 
> (was?) necessary for those targets lacking board_usb_init().

That's what triggered my confusion: there is no sunxi board as such that
implements board_usb_init(), and with our USB PHY code being DT driven, we
should not need it anyway.
There are empty fallback implementations in common/usb.c and
drivers/usb/gadget/g_dnl.c, and we pick up one of them, given the right
Kconfig symbols defined.

I just tried it: enabling the "ums" command, and it worked:

$ make orangepi_zero3_defconfig
$ scripts/config --enable CONFIG_CMD_USB_MASS_STORAGE
$ make olddefconfig
$ make

then, on the U-Boot prompt:
=> ums 0 mmc 0
and the SD card block device popped up on my host.

(That the ums command is useful enough to be enabled by default is a
separate story).
Gadget Ethernet and fastboot are even enabled by default, so whenever you
have a defconfig with CONFIG_USB_MUSB_GADGET=y, you should get this
functionality.

So can anyone tell why this patch is needed so desperately?

> >> better if we just make the sunxi USB driver compatible with the
> >> DM gadget model, as many other musb-new variants already are.
> >>
> >> This change has been verified working on a T113s.
> >>
> >> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> >> ---
> >>   drivers/usb/musb-new/sunxi.c | 50 +++++++++++++++++++++++-------------
> >>   1 file changed, 32 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> >> index 510b254f7d..6658cd995d 100644
> >> --- a/drivers/usb/musb-new/sunxi.c
> >> +++ b/drivers/usb/musb-new/sunxi.c
> >> @@ -444,6 +444,16 @@ static struct musb_hdrc_config musb_config_h3 = {
> >>   	.ram_bits	= SUNXI_MUSB_RAM_BITS,
> >>   };
> >>   
> >> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)  
> > 
> > Please no more #ifdef's. Is there any reason to *not* force
> > DM_USB_GADGET now, for all sunxi boards, in Kconfig?
> > Either by "select"ing it in the USB Kconfig, or in arch/arm/Kconfig, like
> > other platforms do.
> > Then you don't need to care about the !DM_USB_GADGET definition of this
> > function and can drop the #ifdef.  
> 
> I wouldn't be the one to ask. I can't think of any such reason myself. 
> But to me it sounds like since *no sunxi board provides 
> board_usb_init()* the only way USB gadgets *could* work is with 
> DM_USB_GADGET? That'd be reason enough to force it.

See above.
So I think the only reason to *not* have DM_USB_GADGET in general is
because some driver is not ready for that yet, or there is some odd device
that requires some extra hacks that the DM implementation does not cover.

So with this patch we remove those reasons for sunxi, AFAICT. The Allwinner
MUSB driver is now DM ready, so there is no reason to not set it. Think of
it more as we are now declaring the sunxi MUSB driver as DM ready. That's
why I want to see it set unconditionally, as it's not a user choice per se.

Eventually this DM specific symbol will go away at some point, as it did
for other subsystems like Ethernet already.

> >> +int dm_usb_gadget_handle_interrupts(struct udevice *dev) {  
> > 
> > coding style  
> 
> Sentence fragments are harder to understand. I am assuming you are 
> saying, "Please put the opening '{' on its own line."

Funny enough I had these extra words, but then deleted them in fear it
would sound too patronising for this obvious typo ;-)

> >> +	struct sunxi_glue *glue = dev_get_priv(dev);
> >> +	struct musb_host_data *host = &glue->mdata;
> >> +
> >> +	host->host->isr(0, host->host);
> >> +	return 0;
> >> +}
> >> +#endif
> >> +
> >>   static int musb_usb_probe(struct udevice *dev)
> >>   {
> >>   	struct sunxi_glue *glue = dev_get_priv(dev);
> >> @@ -452,10 +462,6 @@ static int musb_usb_probe(struct udevice *dev)
> >>   	void *base = dev_read_addr_ptr(dev);
> >>   	int ret;
> >>   
> >> -#ifdef CONFIG_USB_MUSB_HOST
> >> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> >> -#endif
> >> -
> >>   	if (!base)
> >>   		return -EINVAL;
> >>   
> >> @@ -486,23 +492,31 @@ static int musb_usb_probe(struct udevice *dev)
> >>   	pdata.platform_ops = &sunxi_musb_ops;
> >>   	pdata.config = glue->cfg->config;
> >>   
> >> -#ifdef CONFIG_USB_MUSB_HOST
> >> -	priv->desc_before_addr = true;
> >> +	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
> >> +		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> >> +		priv->desc_before_addr = true;
> >>   
> >> -	pdata.mode = MUSB_HOST;
> >> -	host->host = musb_init_controller(&pdata, &glue->dev, base);
> >> -	if (!host->host)
> >> -		return -EIO;
> >> +		pdata.mode = MUSB_HOST;
> >> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
> >> +		if (!host->host)
> >> +			return -EIO;
> >>   
> >> -	return musb_lowlevel_init(host);
> >> -#else
> >> -	pdata.mode = MUSB_PERIPHERAL;
> >> -	host->host = musb_register(&pdata, &glue->dev, base);
> >> -	if (IS_ERR_OR_NULL(host->host))
> >> -		return -EIO;
> >> +		return musb_lowlevel_init(host);
> >> +	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
> >> +		pdata.mode = MUSB_PERIPHERAL;
> >> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
> >> +		if (!host->host)
> >> +			return -EIO;
> >>   
> >> -	return 0;
> >> -#endif
> >> +		return usb_add_gadget_udc(&glue->dev, &host->host->g);
> >> +	} else {
> >> +		pdata.mode = MUSB_PERIPHERAL;
> >> +		host->host = musb_register(&pdata, &glue->dev, base);
> >> +		if (IS_ERR_OR_NULL(host->host))
> >> +			return -EIO;
> >> +
> >> +		return 0;
> >> +	}  
> > 
> > That looks like a good cleanup! Just need to test it briefly, but it seems
> > like the gist of this patch is fine.  
> 
> I think it would be wise to test it a little better than "briefly" given 
> the age of the patch. I'm not well-equipped to do any testing myself 
> right now or I'd volunteer.

So I gave it a spin on my OPi Zero 3. It seems to work(TM), although I
need to stop the connection to the Ethernet gadget:

=> ums 0 mmc 0
UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x3af000
All UDCs in use (1 available), use the unbind command
g_dnl_register: failed!, error: -19
g_dnl_register failed
=> unbind ethernet 1
=> ums 0 mmc 0
=> ums 0 mmc 0
UMS: LUN 0, dev mmc 0, hwpart 0, sector 0x0, count 0x3af000
(works and waits at this point)

This seems like a quite annoying limitation, if anyone has an idea what
should be done here, I am all ears. Maybe dropping the default Ethernet
connection?

Thanks!
Andre

> > 
> >   
> >>   }
> >>   
> >>   static int musb_usb_remove(struct udevice *dev)  
> >   

Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[John Watts]

Hi,

On Fri, Jun 28, 2024 at 04:17:27PM +0100, Andre Przywara wrote:
> > I do not know if this patch is still a 
> > necessity; though if John is nudging about it, it probably is.
> 
> Yes apparently he needs it, though I am not entirely sure why.
> USB gadget has worked for ages in sunxi, without DM_USB_GADGET support,
> that's why I was a bit puzzled why this patch seems so important.
> 
> And secondly I was put off by John's initial reply that it would trigger
> many USB errors for him. He later rectified it, but I must have missed
> that message.

This patch is necessary for gadget to work on the T113 board I have.
Without it I get this error during fastboot:

Controller uninitialized
g_dnl_register: failed!, error: -6

John.

Re: [PATCH v2 2/2] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Andre Przywara]

On Sat, 29 Jun 2024 02:22:14 +1000
John Watts <contact@jookia.org> wrote:

Hi,

> On Fri, Jun 28, 2024 at 04:17:27PM +0100, Andre Przywara wrote:
> > > I do not know if this patch is still a 
> > > necessity; though if John is nudging about it, it probably is.  
> > 
> > Yes apparently he needs it, though I am not entirely sure why.
> > USB gadget has worked for ages in sunxi, without DM_USB_GADGET support,
> > that's why I was a bit puzzled why this patch seems so important.
> > 
> > And secondly I was put off by John's initial reply that it would trigger
> > many USB errors for him. He later rectified it, but I must have missed
> > that message.  
> 
> This patch is necessary for gadget to work on the T113 board I have.
> Without it I get this error during fastboot:
> 
> Controller uninitialized
> g_dnl_register: failed!, error: -6

So I dug into this, and this is what I think is going on:

If you add "CONFIG_USB_MUSB_GADGET=y" to your *defconfig*, it should
work. If you just enable it via menuconfig or by editing .config, it
will not. Adding CONFIG_USB_ETHER to the mix should fix this.
This symbol is automatically selected in Kconfig when it sees
USB_MUSB_GADGET, but apparently only during a defconfig run, it's not
selected during the other methods. Not sure what causes this difference
in behaviour, but I have seen this in U-Boot before.

That being said: this is of course not a proper solution, the DM
support proposed in that patch is the right way to go, but it should
explain why it works for me, but not for you.

As for this patch: the problem is where the Ethernet gadget code
apparently grabs the UDC device, without actually using it already. This
prevents other gadgets like ums to be used, without unbinding the
Ethernet gadget with a non-obvious command first.
So I think we need a solution for this first, otherwise the user
experience would suffer.

Cheers,
Andre