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)

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)