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)